fix: harden git exec resolution in SourceLink metadata#53
fix: harden git exec resolution in SourceLink metadata#53
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of the build process by hardening the resolution of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Summary by CodeRabbitRelease Notes
WalkthroughThe change replaces implicit PATH-based git discovery with explicit, platform-aware git executable resolution via a new internal Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b02443af77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathRooted('$(GitExe)'))">$(GitExe)</_GitExe> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' != 'Windows_NT' and Exists('/usr/bin/git')">/usr/bin/git</_GitExe> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' != 'Windows_NT' and Exists('/usr/local/bin/git')">/usr/local/bin/git</_GitExe> |
There was a problem hiding this comment.
_GitExe is now only set from a short list of hard-coded absolute paths or an absolute GitExe override. For consumers whose Git is only discoverable through PATH—for example Homebrew/Nix installs on Unix or Scoop/portable installs on Windows—_GitExe stays empty, the rev-parse --show-toplevel call at line 35 is skipped, and this target then flips _ANcpLuaInGitRepo to false. The build still succeeds, but SourceLink and repository metadata are silently dropped for projects that are actually inside a git repo, which is a regression from the previous plain git ... invocations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens the MSBuild SourceLink/git metadata probing to avoid executing a repo-local planted git binary (command-hijack/RCE risk) by resolving git to a trusted absolute path before invoking it.
Changes:
- Introduces a
_GitExeproperty that selects an absolute git executable path (preferring an explicitly provided absolute$(GitExe)). - Replaces
Exec Command="git ..."withExec Command="\"$(_GitExe)\" ..."within_ANcpLuaComputeGitMetadata. - Guards the initial git-root probe to run only when a trusted git path was resolved.
Comments suppressed due to low confidence (4)
src/Build/Enforcement/DeterminismAndSourceLink.props:62
- The later git Exec tasks are guarded by
$(_ANcpLuaInGitRepo)but not by$(_GitExe). If_GitRoot(or_ANcpLuaInGitRepo) is ever pre-set from outside this props/target, these Execs can run with an empty$(_GitExe)(command becomes"" -C ...) and silently skip metadata. Consider addingand '$(_GitExe)' != ''to these ExecConditions (or folding the$(_GitExe)check into the_ANcpLuaInGitRepocomputation) so “no trusted git found” consistently disables all git Exec calls.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" config --get remote.origin.url$(_StderrRedirect)"
ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
StandardErrorImportance="Low"
Condition="'$(_ANcpLuaInGitRepo)' == 'true'">
src/Build/Enforcement/DeterminismAndSourceLink.props:71
- This git Exec is conditioned on
$(_ANcpLuaInGitRepo)but not on$(_GitExe). To keep the “no trusted git found” path consistent (and avoid executing an empty tool path if_GitRootis ever set externally), consider addingand '$(_GitExe)' != ''here as well.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" symbolic-ref --quiet HEAD$(_StderrRedirect)"
ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
StandardErrorImportance="Low"
Condition="'$(_ANcpLuaInGitRepo)' == 'true'">
src/Build/Enforcement/DeterminismAndSourceLink.props:80
- This git Exec is not guarded by
$(_GitExe) != ''. If_ANcpLuaInGitRepobecomes true while_GitExeis empty (e.g., due to an externally supplied_GitRoot), the command will effectively be"" -C ...and silently fail. Addand '$(_GitExe)' != ''to theConditionto ensure all git invocations are disabled when no trusted git path is resolved.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse --abbrev-ref HEAD$(_StderrRedirect)"
ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
StandardErrorImportance="Low"
Condition="'$(_ANcpLuaInGitRepo)' == 'true' and '$(_GitBranchRefRaw)' == ''">
src/Build/Enforcement/DeterminismAndSourceLink.props:89
- This git Exec is conditioned on
$(_ANcpLuaInGitRepo)but not on$(_GitExe). For consistency with the initial probe (and to avoid attempting to execute an empty tool path), addand '$(_GitExe)' != ''to thisCondition(or ensure_ANcpLuaInGitRepocan’t be true when_GitExeis empty).
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse HEAD$(_StderrRedirect)"
ConsoleToMSBuild="true"
IgnoreExitCode="true"
StandardOutputImportance="Low"
StandardErrorImportance="Low"
Condition="'$(_ANcpLuaInGitRepo)' == 'true'">
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe> |
There was a problem hiding this comment.
On Windows, properties like $(ProgramW6432) can be empty (e.g., 32-bit environments). In that case Exists('$(ProgramW6432)\Git\cmd\git.exe') becomes Exists('\Git\cmd\git.exe'), which can unexpectedly match a drive-root path and undermines the intent of selecting a well-known install location. Add guards like '$(ProgramW6432)' != '' (and similarly for other base dirs) before the Exists(...) checks; also consider adding $(ProgramFiles(x86))\Git\cmd\git.exe as another common Git-for-Windows install location.
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramW6432)' != '' and Exists('$(ProgramW6432)\Git\cmd\git.exe')">$(ProgramW6432)\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles(x86))' != '' and Exists('$(ProgramFiles(x86))\Git\cmd\git.exe')">$(ProgramFiles(x86))\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles)' != '' and Exists('$(ProgramFiles)\Git\cmd\git.exe')">$(ProgramFiles)\Git\cmd\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(ProgramFiles)' != '' and Exists('$(ProgramFiles)\Git\bin\git.exe')">$(ProgramFiles)\Git\bin\git.exe</_GitExe> | |
| <_GitExe Condition="'$(_GitExe)' == '' and '$(OS)' == 'Windows_NT' and '$(LocalAppData)' != '' and Exists('$(LocalAppData)\Programs\Git\cmd\git.exe')">$(LocalAppData)\Programs\Git\cmd\git.exe</_GitExe> |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the build process against potential command-hijacking vulnerabilities by resolving the git executable to a trusted, absolute path instead of relying on the system's PATH. The approach of defining a _GitExe property with a fallback mechanism to common installation locations is solid. My review includes suggestions to further improve the robustness of the implementation by ensuring all git command executions are skipped if a trusted git executable cannot be found.
| </PropertyGroup> | ||
|
|
||
| <Exec Command="git -C "$(_GitRoot)" config --get remote.origin.url$(_StderrRedirect)" | ||
| <Exec Command=""$(_GitExe)" -C "$(_GitRoot)" config --get remote.origin.url$(_StderrRedirect)" |
There was a problem hiding this comment.
For consistency with the first Exec task and to improve robustness, it's advisable to add a condition here to ensure this task only runs if _GitExe has been successfully resolved. Without the condition, if _GitExe is empty, MSBuild will attempt to execute an empty command, which will fail. Although IgnoreExitCode="true" prevents a build failure, explicitly skipping the task is a cleaner approach.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" config --get remote.origin.url$(_StderrRedirect)"
Condition="'$(_GitExe)' != ''"
| </Exec> | ||
|
|
||
| <Exec Command="git -C "$(_GitRoot)" symbolic-ref --quiet HEAD$(_StderrRedirect)" | ||
| <Exec Command=""$(_GitExe)" -C "$(_GitRoot)" symbolic-ref --quiet HEAD$(_StderrRedirect)" |
There was a problem hiding this comment.
Similar to my other comments, adding a condition to this Exec task will make the logic more robust by preventing an attempt to run an empty command if a trusted git executable isn't found.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" symbolic-ref --quiet HEAD$(_StderrRedirect)"
Condition="'$(_GitExe)' != ''"
| </Exec> | ||
|
|
||
| <Exec Command="git -C "$(_GitRoot)" rev-parse --abbrev-ref HEAD$(_StderrRedirect)" | ||
| <Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse --abbrev-ref HEAD$(_StderrRedirect)" |
There was a problem hiding this comment.
To maintain consistency and prevent errors from attempting to run an empty command, please add a condition to this Exec task to ensure it only runs when _GitExe is defined.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse --abbrev-ref HEAD$(_StderrRedirect)"
Condition="'$(_GitExe)' != ''"
| </Exec> | ||
|
|
||
| <Exec Command="git -C "$(_GitRoot)" rev-parse HEAD$(_StderrRedirect)" | ||
| <Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse HEAD$(_StderrRedirect)" |
There was a problem hiding this comment.
As with the other git commands in this target, this Exec task should also be conditional on _GitExe being set. This will prevent it from trying to execute an empty command if git is not found in one of the trusted locations.
<Exec Command=""$(_GitExe)" -C "$(_GitRoot)" rev-parse HEAD$(_StderrRedirect)"
Condition="'$(_GitExe)' != ''"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Build/Enforcement/DeterminismAndSourceLink.props`:
- Line 25: The condition that sets _GitExe from $(GitExe) should require a truly
fully-qualified path and verify the file exists; replace the Path.IsPathRooted
check with Path.IsPathFullyQualified when available (or a regex check such as
^[A-Za-z]:\\|^\\\\ for .NET Framework) and add an Exists check so the assignment
only occurs if the path is fully qualified and File.Exists('$(GitExe)') is true;
update the conditional that references _GitExe and GitExe to use these stricter
validations to avoid accepting drive-relative paths like C:git.exe or stale
overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccd78645-0dfb-46b0-b608-6706130a0f42
📒 Files selected for processing (1)
src/Build/Enforcement/DeterminismAndSourceLink.props
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
**/*.props
⚙️ CodeRabbit configuration file
MSBuild property files (Directory.Build.props, Directory.Packages.props, Version.props). Review for: Central Package Management correctness, version consistency, and that new packages are added with explicit version pins. Flag transitive dependency promotions that aren't justified. Verify TFM targeting is correct (.NET 10).
Files:
src/Build/Enforcement/DeterminismAndSourceLink.props
| <_StderrRedirect Condition="'$(OS)' != 'Windows_NT'"> 2>/dev/null</_StderrRedirect> | ||
|
|
||
| <!-- Resolve git to a trusted absolute path so repo-local binary planting cannot hijack Exec. --> | ||
| <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathRooted('$(GitExe)'))">$(GitExe)</_GitExe> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MSBuild property functions on the supported SDK/Visual Studio versions, does System.IO.Path.IsPathRooted("C:git.exe")return true, and isSystem.IO.Path.IsPathFullyQualified available for enforcing a truly absolute path?
💡 Result:
-
Yes. On Windows,
System.IO.Path.IsPathRooted("C:git.exe")returns true becauseIsPathRootedtreats “drive letter + colon” as “rooted” (even though it’s still drive-relative, not truly absolute). The .NET docs forIsPathFullyQualifiedshow the same behavior with"C:Documents": Rooted = True, Fully qualified = False. [1], [2] -
System.IO.Path.IsPathFullyQualified(...)exists on modern .NET / .NET Standard (not .NET Framework) (the API is documented as coming fromnetstandard.dll/System.Runtimeand is intended to distinguish"C:a"vs"C:\\a"). [1]
MSBuild property functions can call public static .NET methods, but only those available in the runtime hosting MSBuild. [3] -
Practical upshot for “supported SDK/VS versions”:
- If you build with the .NET SDK MSBuild (for example via
dotnet msbuild/dotnet build), you can usePath.IsPathFullyQualifiedto enforce a truly absolute path. [4] - If you build with MSBuild running on .NET Framework (common for Visual Studio’s in-proc/toolset MSBuild),
Path.IsPathFullyQualifiedis not available, so calling it in a property function will fail at evaluation time (you’ll need an alternate check, e.g., pattern-testing for^[A-Za-z]:[\\/]|^\\\\). [1], [3]
- If you build with the .NET SDK MSBuild (for example via
Sources: [1] [2] [3] [4]
Require $(GitExe) to be fully qualified and to exist before trusting it.
Path.IsPathRooted accepts drive-relative Windows paths like C:git.exe (rooted, not fully qualified), which can resolve relative to the current directory and weakens this hardening. The check also short-circuits fallback probes even when the file doesn't exist, silently disabling git metadata on a stale override.
Use Path.IsPathFullyQualified on .NET SDK MSBuild (e.g., dotnet build), or implement an equivalent check for .NET Framework MSBuild. Always pair with Exists validation.
Suggested change
- <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathRooted('$(GitExe)'))">$(GitExe)</_GitExe>
+ <_GitExe Condition="'$(_GitExe)' == '' and '$(GitExe)' != '' and $([System.IO.Path]::IsPathFullyQualified('$(GitExe)')) and Exists('$(GitExe)')">$(GitExe)</_GitExe>For .NET Framework MSBuild, replace IsPathFullyQualified with a pattern check or equivalent validation ensuring the path is truly absolute (e.g., ^[A-Za-z]:\\|^\\\\).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Build/Enforcement/DeterminismAndSourceLink.props` at line 25, The
condition that sets _GitExe from $(GitExe) should require a truly
fully-qualified path and verify the file exists; replace the Path.IsPathRooted
check with Path.IsPathFullyQualified when available (or a regex check such as
^[A-Za-z]:\\|^\\\\ for .NET Framework) and add an Exists check so the assignment
only occurs if the path is fully qualified and File.Exists('$(GitExe)') is true;
update the conditional that references _GitExe and GitExe to use these stricter
validations to avoid accepting drive-relative paths like C:git.exe or stale
overrides.
|
@claude review the suggestions and implemented fixes merge when all done |
Motivation
_ANcpLuaComputeGitMetadataMSBuild target insrc/Build/Enforcement/DeterminismAndSourceLink.propsinvokedgitby name viaExec, enabling repository-local binary planting and a build-time command-hijack RCE vector.Description
_GitExeproperty that prefers an absolute$(GitExe)when provided and otherwise falls back to common absolute git install locations on Unix and Windows to ensure a trusted executable path.gitExecinvocations in the_ANcpLuaComputeGitMetadatatarget with calls to"$(_GitExe)"and guard the initial probe withCondition="'$(_GitExe)' != ''", preserving the non-git fallback behavior when no trusted git is found.IgnoreExitCodebehavior so metadata probing remains best-effort and existing warnings/fallbacks are unchanged.Testing
rg -n 'Exec Command="git 'againstsrc/Build/Enforcement/DeterminismAndSourceLink.propsand verified there are no remaining baregitExecusages (success).sed/nlto confirm the_GitExeproperty and replacements were applied to allExeclines in the target (success).command -v dotnetandcommand -v msbuildto perform a build/test run but both tools are not available in this environment, so nodotnet/MSBuild test run could be executed (not executed).Codex Task